HYPERFLEET-1328 - refactor: simplify resource handlers and add registry cycle detection#286
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughResource handlers now centralize parent and ownership checks for flat and nested routes. Child creation without parent context returns a validation error with nested-route guidance and HTTP 400. Root resource creation resolves parent and child paths from registry descriptors. Middleware tests cover root resource matching, and registry validation detects Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RootResourceHandler
participant Registry
Client->>RootResourceHandler: POST child kind at root route
RootResourceHandler->>Registry: Resolve parent descriptor
Registry-->>RootResourceHandler: Return parent plural
RootResourceHandler-->>Client: Return HTTP 400 validation guidance
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/registry/registry.go (1)
109-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge duplicate
ParentKind != ""guards and extract cycle check.Two separate
if d.ParentKind != ""blocks run back-to-back (Lines 109 and 118). Combine them and extract the traversal into a helper (e.g.checkParentKindCycle(d, descriptors)) —Validate()is now ~66 lines with well over 5 independent panic branches, exceeding the decomposition threshold.♻️ Proposed refactor
- if d.ParentKind != "" { - if _, ok := descriptors[d.ParentKind]; !ok { - panic(fmt.Sprintf( - "entity kind %q references unregistered parent kind %q", - d.Kind, d.ParentKind, - )) - } - } - - if d.ParentKind != "" { - visited := map[string]bool{d.Kind: true} - for cur := d.ParentKind; cur != ""; { - if visited[cur] { - panic(fmt.Sprintf( - "ownership cycle detected: kind %q participates in a ParentKind cycle", - d.Kind, - )) - } - visited[cur] = true - cur = descriptors[cur].ParentKind - } - } + if d.ParentKind != "" { + if _, ok := descriptors[d.ParentKind]; !ok { + panic(fmt.Sprintf( + "entity kind %q references unregistered parent kind %q", + d.Kind, d.ParentKind, + )) + } + checkParentKindCycle(d, descriptors) + }As per path instructions, "Functions >50 lines or >5 branching paths — flag for decomposition."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/registry/registry.go` around lines 109 - 131, In Validate, merge the duplicated ParentKind presence checks into a single guard and move the ancestry traversal into a helper such as checkParentKindCycle. Use the existing descriptors map and d.Kind/d.ParentKind in the helper to preserve the same panic behavior, but keep Validate focused by delegating the cycle detection logic instead of having two back-to-back branches.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/handlers/resource_handler.go`:
- Around line 45-63: The child-kind create rejection is currently using
errors.Validation(), which maps to 400 instead of the expected 422 response.
Update the shared child-create validation logic used by ResourceHandler and
RootResourceHandler so both the flat and nested create paths return
Unprocessable Entity for invalid child-kind creates, preserving the API contract
expected by the root_resources integration tests. Locate the duplicated
rejection code in ResourceHandler and RootResourceHandler and route both through
the same helper that produces the 422 response.
In `@pkg/handlers/root_resource_handler.go`:
- Around line 92-97: The child-kind root create path in rootResourceHandler
should return the dedicated 422 contract instead of the generic validation
status. In the branch that checks descriptor.ParentKind and builds the “create
it via /{id}/...” message, either set the HTTPCode explicitly to 422 on the
returned error or switch to the existing child-create error helper so POST
/api/hyperfleet/v1/resources preserves the child-create behavior.
---
Nitpick comments:
In `@pkg/registry/registry.go`:
- Around line 109-131: In Validate, merge the duplicated ParentKind presence
checks into a single guard and move the ancestry traversal into a helper such as
checkParentKindCycle. Use the existing descriptors map and d.Kind/d.ParentKind
in the helper to preserve the same panic behavior, but keep Validate focused by
delegating the cycle detection logic instead of having two back-to-back
branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 992f9348-4315-439a-9194-d319079ce1e2
📒 Files selected for processing (7)
pkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/handlers/root_resource_handler.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_match_test.gopkg/registry/registry.gopkg/registry/registry_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
Risk Score: 1 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 324 lines (>200) | +1 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
1cba700 to
2fe4a11
Compare
2fe4a11 to
6b5632f
Compare
|
/retest |
6b5632f to
c75274e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/handlers/resource_handler_test.go (1)
845-847: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the actionable nested-route error detail.
This test passes even if the handler returns the wrong or empty message. Verify the problem-detail body includes guidance such as
/channels/{id}/versions, since that message is part of the behavior being changed.🧪 Proposed assertion
Expect(rr.Code).To(Equal(http.StatusBadRequest)) + Expect(rr.Body.String()).To(ContainSubstring(`/channels/{id}/versions`))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/handlers/resource_handler_test.go` around lines 845 - 847, The test around handler.Create must validate the response body, not only the HTTP status. After asserting http.StatusBadRequest, inspect the problem-detail response and verify it contains actionable nested-route guidance such as “/channels/{id}/versions”.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/handlers/resource_handler_test.go`:
- Around line 831-833: Restore the process-global registry after the test: in
the test containing the registry.Reset and registry.Register calls, capture the
existing registry state before mutation and register a cleanup handler to
restore it afterward; if snapshot/restore is unavailable, use test cleanup to
reset and re-register the original descriptors, and ensure the test does not run
in parallel.
---
Nitpick comments:
In `@pkg/handlers/resource_handler_test.go`:
- Around line 845-847: The test around handler.Create must validate the response
body, not only the HTTP status. After asserting http.StatusBadRequest, inspect
the problem-detail response and verify it contains actionable nested-route
guidance such as “/channels/{id}/versions”.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0ac45817-4e09-416f-8eda-3e0c96c4dc38
📒 Files selected for processing (8)
pkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/handlers/root_resource_handler.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_match_test.gopkg/registry/registry.gopkg/registry/registry_test.gotest/integration/root_resources_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
✅ Files skipped from review due to trivial changes (1)
- pkg/middleware/schema_validation.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/handlers/root_resource_handler.go
- pkg/registry/registry_test.go
- pkg/registry/registry.go
- test/integration/root_resources_test.go
- pkg/middleware/schema_validation_match_test.go
- pkg/handlers/resource_handler.go
c75274e to
7ea3eb3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/handlers/resource_handler.go`:
- Around line 84-90: Remove the checkParentExists call from the Get and List
handlers. Branch directly on whether parent_id is present, then invoke
GetByOwner or ListByOwner so those methods determine the missing-parent
behavior, preserving an empty collection for List and the appropriate not-found
result for Get.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e614aa53-3aa3-4b42-8d42-911a956651e9
📒 Files selected for processing (8)
pkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/handlers/root_resource_handler.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_match_test.gopkg/registry/registry.gopkg/registry/registry_test.gotest/integration/root_resources_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
✅ Files skipped from review due to trivial changes (1)
- pkg/middleware/schema_validation.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/handlers/root_resource_handler.go
- pkg/middleware/schema_validation_match_test.go
- pkg/registry/registry.go
- pkg/handlers/resource_handler_test.go
- pkg/registry/registry_test.go
- test/integration/root_resources_test.go
7ea3eb3 to
f361d3f
Compare
…ry cycle detection Consolidate parent-kind checks in resource handlers with actionable error messages, add ParentKind cycle detection to registry startup validation, and harden schema validation path matching.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/root_resources_test.go`:
- Line 168: Align the child-create status code contract across the handler,
integration tests, and documentation: use 422 Unprocessable Entity for the
/resources path, updating the assertions near the affected tests and the
corresponding handler response and docs while preserving other status-code
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ad577cfb-c888-4f45-a954-2cdbbcc7af60
📒 Files selected for processing (7)
pkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/middleware/schema_validation.gopkg/middleware/schema_validation_match_test.gopkg/registry/registry.gopkg/registry/registry_test.gotest/integration/root_resources_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/registry/registry_test.go
- pkg/middleware/schema_validation_match_test.go
- pkg/registry/registry.go
- pkg/handlers/resource_handler_test.go
- pkg/handlers/resource_handler.go
|
@kuudori: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
f361d3f to
c309ee6
Compare
5 participants
Summary
ResourceHandlerandRootResourceHandlerwith actionable error messages that point to the exact nested route (e.g.create it via /channels/{id}/versions)ParentKindcycle detection toregistry.Validate()so misconfigured ownership chains are caught at startup instead of causing infinite recursion at runtimeBehavior change: parent-existence pre-check removed from Get/List
GET /channels/{bad-id}/versions/{v-id}GET /channels/{bad-id}/versionsTest plan
TestValidate_ParentKindCycle_Panics)go vetclean across all modified packages